Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Remove restraint of vtk<9.1.0 #132

Merged
merged 18 commits into from
Aug 19, 2022
Merged

Remove restraint of vtk<9.1.0 #132

merged 18 commits into from
Aug 19, 2022

Conversation

PProfizi
Copy link
Contributor

Solves #107

@PProfizi PProfizi added the bug Something isn't working label Aug 11, 2022
@PProfizi PProfizi self-assigned this Aug 11, 2022
@codecov
Copy link

codecov bot commented Aug 11, 2022

Codecov Report

Merging #132 (dcabaab) into master (2142edc) will increase coverage by 0.01%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #132      +/-   ##
==========================================
+ Coverage   82.80%   82.81%   +0.01%     
==========================================
  Files          26       26              
  Lines        1407     1408       +1     
==========================================
+ Hits         1165     1166       +1     
  Misses        242      242              

@PProfizi
Copy link
Contributor Author

Windows fatal exception: access violation

Thread 0x000006e8 (most recent call first):
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pyvista\plotting\plotting.py", line 1470 in renderWindows fatal exception:
access violation File

"C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\pyvista\plotting\plotting.py", line 5533 in show
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\ansys\dpf\core\plotter.py", line 780 in plot_contour
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\site-packages\ansys\dpf\post\result_data.py", line 352 in plot_contour
File "<doctest post.result_data.ResultData.plot_contour[5]>", line 1 in
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\doctest.py", line 1334 in __run
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\doctest.py", line 1481 in run
File "C:\hostedtoolcache\windows\Python\3.9.13\x64\lib\doctest.py"D:\a_temp\aaa44f51-9ba7-41ce-b862-69b1d7ff185f.sh: line 3: 58 Segmentation fault pytest --doctest-modules $DEBUG --junitxml=junit/test-doctests-results.xml ansys/dpf/post
ansys/dpf/post/result_data.py::post.result_data.ResultData.plot_contour
Error: Process completed with exit code 139.

@RobPasMue
Copy link
Member

I am guessing your VTK dependency is because of PyVista right? Are you using VTK explicitly in some place? I think @akaszynski can weigh in better than anybody here. I know that VTK (as of released packages - i.e. not beta versions or release candidates) is not yet supported on Python 3.10 and they have some PyVista wheels for this specific case.. He'll probably have more info than anyone else.

@PProfizi
Copy link
Contributor Author

Indeed, we do not use vtk directly, only through PyVista.
So what you are saying is that when using pyvista on Python 3.10 we cannot simply pip install?

Here the problem is not about Python 3.10, but when using vtk==9.1.0 for Pythons 3.7 to 3.9, where we get a memory error on Windows, although I could not replicate the issue locally.
I am guessing it has something to do with the specific windows installation of the github runners.

@RobPasMue
Copy link
Member

No, what I meant was that @akaszynski provided some specific VTK wheels for 9.1.0 and Python 3.10 in the PyVista project, as far as I am aware of. Follow the link: pyvista/pyvista#2064

I am seeing though you are having problems with other Python versions now... The output on the runners is limited though. Are you getting more info locally? Or does it work just fine? Try running everything from a clean virtual env. Maybe that gives you more info on the error. Though I am sure you must have tried it already.

@akaszynski
Copy link
Contributor

This issue occurs on Windows when using the OSMesa opengl on windows. This shouldn't be an issue on linux.

@akaszynski
Copy link
Contributor

akaszynski commented Aug 11, 2022

Indeed, we do not use vtk directly, only through PyVista. So what you are saying is that when using pyvista on Python 3.10 we cannot simply pip install?

Here the problem is not about Python 3.10, but when using vtk==9.1.0 for Pythons 3.7 to 3.9, where we get a memory error on Windows, although I could not replicate the issue locally. I am guessing it has something to do with the specific windows installation of the github runners.

I'm going to have to review and improve patch this.

@PProfizi
Copy link
Contributor Author

We are using a pyvista github action to prepare the headless display in our CI. (pyvista/setup-headless-display-action@v1)
For anyone else other than Alex, this indeed means this script is used which installs a MesaOpenGL opengl32_mingw_64.dll for rendering.

@PProfizi PProfizi linked an issue Aug 16, 2022 that may be closed by this pull request
@PProfizi
Copy link
Contributor Author

Hi @akaszynski, any updates on this issue?

@akaszynski
Copy link
Contributor

Hi @akaszynski, any updates on this issue?

On my (growing) todo list.

@akaszynski akaszynski changed the title Remove restrain on vtk<9.1.0 Remove restraint of vtk<9.1.0 Aug 18, 2022
@akaszynski
Copy link
Contributor

Fix was simply to install vtk<=9.1 for windows. There's an issue with OSMesa for VTK on Windows with vtk>=9.1.0 and this is very rarely used by users (batch processing is on linux typically), so I see no issue with just installing the older version until this is fixed upstream.

akaszynski
akaszynski previously approved these changes Aug 18, 2022
Copy link
Contributor

@akaszynski akaszynski left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM still needs to be fixed as <9.1 isn't available for Py 3.10 on Windows.

@akaszynski akaszynski dismissed their stale review August 18, 2022 20:00

still needs to be fixed as <9.1 isn't available for Py 3.10 on Windows.

@akaszynski
Copy link
Contributor

Identified the minimum reproducible error:
pyvista/pyvista#3185

Copy link
Member

@RobPasMue RobPasMue left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM!

@@ -121,7 +121,7 @@ jobs:
strategy:
fail-fast: false
matrix:
python-version: ["3.8", "3.9"]
python-version: ["3.8", "3.9", "3.10"]
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
python-version: ["3.8", "3.9", "3.10"]
python-version: ["3.7", "3.8", "3.9", "3.10"]

I think that, since we are supporting above Python 3.7, shouldn't it be included in this matrix testing?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is tested in job Build_and_Test. We separated them to be able to trigger them independantly. The CI will however be reworked soon with a testing strategy coherent with what is being put in place for PyDPF-Core.

@PProfizi
Copy link
Contributor Author

Tried using the same trick for doctest, yet it does not work for Python 3.10 (see here).
Keeping the skip of doctest for plot_contour for now.

@PProfizi PProfizi merged commit bbd5125 into master Aug 19, 2022
@PProfizi PProfizi deleted the fix/support_vtk_above_9_1_0 branch August 19, 2022 08:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Support for Python 3.10
3 participants